Skip to content

Fix path parsing on Windows in SymbolIconProviderRegistry & add tests#1532

Merged
rubenporras merged 2 commits into
eclipse-lsp4e:mainfrom
travkin79:patch-add-icon-provider-test
May 6, 2026
Merged

Fix path parsing on Windows in SymbolIconProviderRegistry & add tests#1532
rubenporras merged 2 commits into
eclipse-lsp4e:mainfrom
travkin79:patch-add-icon-provider-test

Conversation

@travkin79
Copy link
Copy Markdown
Contributor

On Windows a path parsing issue can occur while retrieving overlay icons for the outline view, type hierarchy, or call hierarchy. This PR provides a fix for an issue reported in CDT LSP and adds a test for reproducing that issue.

@travkin79 travkin79 changed the title Fix path parsing on Windows in SymbolIconProvider + Tests Fix path parsing on Windows in SymbolIconProvider & add tests Apr 27, 2026
@FlorianKroiss FlorianKroiss self-requested a review April 27, 2026 15:17
@travkin79 travkin79 requested a review from FlorianKroiss April 28, 2026 08:15
@travkin79 travkin79 marked this pull request as ready for review April 28, 2026 08:15
@travkin79 travkin79 changed the title Fix path parsing on Windows in SymbolIconProvider & add tests Fix path parsing on Windows in SymbolIconProviderRegistry & add tests Apr 28, 2026
- Add test case for reproducing a windows path parsing issue
- Move helper method to utility class
- Fix path parsing: Path.of(uri.getPath()) => Path.of(uri)
- Bump test plug-in version
@travkin79 travkin79 force-pushed the patch-add-icon-provider-test branch from 973e19c to b2f851e Compare April 30, 2026 09:42
@travkin79
Copy link
Copy Markdown
Contributor Author

Thank you @FlorianKroiss for your review.

Hello @rubenporras, do you think, we could merge now? I squashed and rebased my PR commits. Oh, I see, the license check fails, but I don't see the reason. Any suggestions?

Besides, I'm wondering about some of our tests sporadically failing for strange reasons. Are there any ideas how we could avoid that?

@rubenporras
Copy link
Copy Markdown
Contributor

Thank you @FlorianKroiss for your review.

Hello @rubenporras, do you think, we could merge now? I squashed and rebased my PR commits.

I have just a minor comment, could you take a look and follow my suggestion or comment if you disagree? Then we can merge. Sorry for the late review, I have been on holidays for a week and forgot to set my status on GitHub, so it was not visible.

Oh, I see, the license check fails, but I don't see the reason. Any suggestions?

@travkin79 , I have triggered the job again and it succeded. I must have been a temporal glitch.

Besides, I'm wondering about some of our tests sporadically failing for strange reasons. Are there any ideas how we could avoid that?

I have been wondering as well but I have currently not much time to work on LSP4E so I cannot help.

@travkin79 travkin79 force-pushed the patch-add-icon-provider-test branch from 1f1c368 to 6a27c5d Compare May 5, 2026 15:20
@travkin79
Copy link
Copy Markdown
Contributor Author

Hi @rubenporras,

I have just a minor comment, could you take a look and follow my suggestion or comment if you disagree? Then we can merge. Sorry for the late review, I have been on holidays for a week and forgot to set my status on GitHub, so it was not visible.

I agree with your suggestion and adapted the code accordingly.

Sorry for the late review, I have been on holidays for a week and forgot to set my status on GitHub, so it was not visible.

No problem.

@rubenporras rubenporras merged commit b912717 into eclipse-lsp4e:main May 6, 2026
11 of 12 checks passed
@travkin79
Copy link
Copy Markdown
Contributor Author

Thank you @rubenporras.

@rubenporras
Copy link
Copy Markdown
Contributor

fyi: I have also created a new release

travkin79 added a commit to travkin79/cdt-lsp that referenced this pull request May 6, 2026
ghentschke pushed a commit to eclipse-cdt/cdt-lsp that referenced this pull request May 6, 2026
…608)

* Customize C/C++ destructor symbol overlay icons in outline view

* Increase min. LSP4E version because of required LSP4E extension point

We depend on the new LSP4E extension point
"org.eclipse.lsp4e.symbolIconsProvider" that was introduced in
org.eclipse.lsp4e vers. 0.19.10 (LSP4E 0.30.3).

* Add test case for custom symbol icon provider

* Do not register custom icon provider for C

Only C++ needs to be considered here

* Cache destructor overlay icon

* Remove unused method

* Add given, when, then comments to test

* Increase min. LSP4E version in order to include LSP4E bug fix

Include PR eclipse-lsp4e/lsp4e#1532
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants